Add support for SQLAlchemy 2.0 #249

rbuffat · 2023-02-05T19:12:30Z

The aim of this PR is to add support for SQLAlchemy 2.0 Declarative Mapping. However, I'm neither familiar with the codebase of sqlacodegen nor the new SQLAlchemy 2.0 style. Thus, it might take time until this PR will be finished (if at all).

Thus, this PR is primarily is mainly created to make this work public for other people that want to contribute or build upon it.

src/sqlacodegen/generators.py

@@ -241,10 +243,9 @@ def add_import(self, obj: Any) -> None:

            if type_.__name__ in dialect_pkg.__all__:
                pkgname = dialect_pkgname
-        elif type_.__name__ in sqlalchemy.__all__:  # type: ignore[attr-defined]


for more information, see https://pre-commit.ci

rbuffat · 2023-03-02T21:29:01Z

@agronholm If you have time, it would be great if you could have a look at this PR.

The current state is that it implements the declarative, table and dataclasses generator for sqlachemy2. However, I need to test the generated models yet with a real database. The sqlmodel generator looks a bit incomplete (e.g. manytomany Relationships). Also sqlmodel seems to be not compatible with Sqlalchemy 2.0. Thus, I would rather not spend time on it.

There are some known issues:
Currently, the mapped_column includes the Column type. This would not be necessary. However, I'm not yet sure how well this works with type checker.

I marked some tests to require Python 3.9, as with this version standard collections can be used as type hints. Theoretically, adding from __future__ import annotations should allow this for older Python versions, but then validate_code() fails. Not yet sure why.

amacfie · 2023-03-02T22:09:56Z

I tested on this (with the dataclasses generator) and that issue seems to be fixed. (So I'm doubly looking forward to this being merged.) The output was this:

from typing import Optional

from sqlalchemy import ForeignKeyConstraint, PrimaryKeyConstraint, Uuid, text
from sqlalchemy.orm import DeclarativeBase, Mapped, MappedAsDataclass, mapped_column, relationship
import uuid

class Base(MappedAsDataclass, DeclarativeBase):
    pass


class Document(Base):
    __tablename__ = 'document'
    __table_args__ = (
        ForeignKeyConstraint(['image_id'], ['document.id'], ondelete='CASCADE', name='image_fk'),
        PrimaryKeyConstraint('id', name='document_pkey')
    )

    id: Mapped[uuid.UUID] = mapped_column(Uuid, primary_key=True, server_default=text('gen_random_uuid()'))
    image_id: Mapped[Optional[uuid.UUID]] = mapped_column(Uuid)

    image: Mapped['Document'] = relationship('Document', remote_side=[id], back_populates='image_reverse')
    image_reverse: Mapped[list['Document']] = relationship('Document', remote_side=[image_id], back_populates='image')

A nitpick, but import uuid should be in the previous paragraph. Also, I'm curious if we need the PrimaryKeyConstraint since there's primary_key=True? I prefer Foo | None to Optional[Foo] but that's not a big deal.

agronholm · 2023-03-03T13:57:37Z

I just noticed the change from list to List, but with from __future__ import annotations, you can keep using list, yes?

rbuffat · 2023-03-03T14:25:28Z

@agronholm I first tried this approach but the test fails in exec() of validate_code(). See this branch: https://github.com/rbuffat/sqlacodegen/tree/try_annotations and the:

Error:

The above exception was the direct cause of the following exception:
tests/test_generator_declarative2.py:631: in test_manytomany_nobidi
    validate_code(
tests/conftest.py:16: in validate_code
    exec(generated_code, {})
<string>:10: in <module>
    ???
/opt/hostedtoolcache/Python/3.9.16/x64/lib/python3.9/site-packages/sqlalchemy/orm/decl_api.py:838: in __init_subclass__
    _as_declarative(cls._sa_registry, cls, cls.__dict__)
/opt/hostedtoolcache/Python/3.9.16/x64/lib/python3.9/site-packages/sqlalchemy/orm/decl_base.py:248: in _as_declarative
    return _MapperConfig.setup_mapping(registry, cls, dict_, None, {})
/opt/hostedtoolcache/Python/3.9.16/x64/lib/python3.9/site-packages/sqlalchemy/orm/decl_base.py:329: in setup_mapping
    return _ClassScanMapperConfig(
/opt/hostedtoolcache/Python/3.9.16/x64/lib/python3.9/site-packages/sqlalchemy/orm/decl_base.py:563: in __init__
    self._scan_attributes()
/opt/hostedtoolcache/Python/3.9.16/x64/lib/python3.9/site-packages/sqlalchemy/orm/decl_base.py:1005: in _scan_attributes
    collected_annotation = self._collect_annotation(
/opt/hostedtoolcache/Python/3.9.16/x64/lib/python3.9/site-packages/sqlalchemy/orm/decl_base.py:1233: in _collect_annotation
    extracted = _extract_mapped_subtype(
/opt/hostedtoolcache/Python/3.9.16/x64/lib/python3.9/site-packages/sqlalchemy/orm/util.py:2277: in _extract_mapped_subtype
    raise sa_exc.ArgumentError(
E   sqlalchemy.exc.ArgumentError: Could not interpret annotation Mapped[int].  Check that it uses names that are correctly imported at the module level. See chained stack trace for more hints.

The Sqlalchemy 1.4 code also uses typing.List. I assume it should be possible to upgrade the generated code using pyupgrade to the required Python version. Thus it should be ok to generate code in the style of the oldest by the project supported Python version.

agronholm · 2023-03-04T11:09:12Z

You need to merge from master, resolve the conflicts and fix the pre-commit errors.

tests/conftest.py

coveralls · 2023-03-04T19:41:53Z

Coverage: 97.564% (+0.1%) from 97.425% when pulling f5b6947 on rbuffat:sqlalchemy2 into 2828e46 on agronholm:master.

agronholm · 2023-03-04T19:58:35Z

pyproject.toml

    "mysql-connector-python",
+]


Suggested change

"mysql-connector-python",

]

"mysql-connector-python",

"sqlacodegen[sqlmodel]",

]

This will cause the sqlmodel tests to actually run, and restore the lost coverage.

@agronholm sqlmodel is not compatible with sqlalchemy 2.0, this makes it tricky.

Oh right. Hmm, those tests need to be run somehow.

I think you had the right idea, but couldn't you just have compared the sqlalchemy version to the string 1.4.* and conditionally installed sqlmodel then?

There are many roads that lead to Rome. This way has the advantage that if one wants to pin SQLalchemy to a certain version only the install line needs to be changed.

I'm not going to want to pin it to a specific version.

This seems resolved to me since bbfacf7, see lines 24-29

for more information, see https://pre-commit.ci

rbuffat · 2023-03-04T20:19:10Z

You need to merge from master, resolve the conflicts and fix the pre-commit errors.

The pre-commit check seemed broken, thus I didn't pay too much attention. It still seems to return different errors compared to what is in master, thus not sure how stable these checks are.

agronholm · 2023-03-04T20:31:00Z

I updated the pre-commit modules this Monday in master.

agronholm · 2023-03-04T20:38:23Z

.github/workflows/test.yml

-        COVERALLS_FLAG_NAME: ${{ matrix.test-name }}
-        COVERALLS_PARALLEL: true
-        SQLALCHEMY_WARN_20: "true"
+      - uses: actions/checkout@v3


I only just noticed: what caused the reformatting? That makes it hard to see the actual changes.

If you could undo the formatting changes and only leave the actual changes in, then I'm ready to merge.

Formatting removed in #275

agronholm · 2023-03-05T12:40:13Z

pyproject.toml

@@ -75,7 +78,8 @@ strict = true
 plugins = ["sqlalchemy.ext.mypy.plugin"]

 [tool.pytest.ini_options]
-addopts = "-rsx --tb=short"
+pythonpath = "src/sqlacodegen"


This is unnecessary.

Removed in #275

agronholm · 2023-03-05T12:45:15Z

pyproject.toml

@@ -75,7 +78,8 @@ strict = true
 plugins = ["sqlalchemy.ext.mypy.plugin"]

 [tool.pytest.ini_options]
-addopts = "-rsx --tb=short"
+pythonpath = "src/sqlacodegen"
+addopts = "-rsx --tb=short --import-mode=importlib"


Why --import-mode=importlib?

Removed in #275

pytest recommends to use importlib: https://docs.pytest.org/en/7.1.x/explanation/goodpractices.html

agronholm · 2023-03-17T10:00:10Z

I will soon resume work on this project, and would like to incorporate this PR ASAP. I have just a few questions/comments (above).

rbuffat · 2023-03-21T19:15:50Z

@agronholm I found some bugs, e.g. determining the type of collections. Moreover, after reflecting for some time, I think a major refactoring is needed to make the different generators independent, as currently, the code is getting unnecessarily complex.

I assume I will have time in the coming weeks to work more on the code.

tomeszmh · 2023-05-15T09:10:47Z

Any news on this one?

Thanks

agronholm · 2023-05-15T10:44:52Z

Not from my side. This project is pretty low on my priority list.

mhauru · 2023-07-19T10:46:53Z

Hello @rbuffat and @agronholm! Our project depends on sqlacodegen (thanks for your work on it!) and we would like to upgrate to SQLAlchemy 2 for various reasons. Judging from the conversation above, this PR seems close to being mergeable. We could put some time into helping get this over the line, but for that it would be very helpful if you could summarise what is still needed. I see some comments by @agronholm above that seem relatively straight-forward, but @rbuffat also mentioned some bugs that they found. Is there a description of them somewhere? Also, there was a mention of a major refactor, that sounds to me like it would be a separate PR and not necessary to finish this, is that correct?

agronholm · 2023-08-01T10:02:10Z

I want this done too, but I've been busy with other projects. I still have one more project that I need to complete the next milestone on before I switch my attention to this one. As such, I welcome any efforts on this PR, but I can't devote any time on it just yet.

mhauru · 2023-08-02T11:05:18Z

I made a commit addressing the comments you left @agronholm, see #275.

I think that covers all the explicit changes you requested. I'm a bit bothered by @rbuffat mentioning he found some bugs, but the tests don't pick them up and if @rbuffat isn't around to tell us more, I'm not sure what to do about them.

rbuffat · 2023-08-02T11:49:50Z

@mhauru As far as I remember, there are issues with name clashes between column names and imports. E.g. a column named uuid.

import uuid
from sqlalchemy import Uuid

class Test(Base):
   uuid: Mapped[uuid.UUID] = mapped_column(Uuid)

Additionally, there are issues with identifying the correct types e.g. for sqlalchemy.dialects.postgresql.JSONB

But the biggest issue is the current architecture. The different generators inherit from each other. With the current changes to support sqlalchemy 2.0 this increases the number of possible paths that can be taken and makes everything in my opinion unmaintainable. I would suggest making the generator not inheriting from each other and moving common functionality (e.g. determining Python types, field naming, importing of libraries out of the individual generators.

agronholm · 2023-08-03T10:26:16Z

Superseded by #275.

rbuffat force-pushed the sqlalchemy2 branch 2 times, most recently from d25ccbc to 388ce49 Compare February 5, 2023 20:22

rbuffat marked this pull request as draft February 6, 2023 16:35

amacfie-tc reviewed Feb 25, 2023

View reviewed changes

src/sqlacodegen/generators.py Outdated

@@ -241,10 +243,9 @@ def add_import(self, obj: Any) -> None:

if type_.__name__ in dialect_pkg.__all__:

pkgname = dialect_pkgname

elif type_.__name__ in sqlalchemy.__all__: # type: ignore[attr-defined]

This comment was marked as outdated.

Sign in to view

rbuffat added 3 commits March 1, 2023 23:12

add support for sqlalchemy2

aeced78

Merge remote-tracking branch 'upstream/master' into sqlalchemy2

d6cdc9e

support sqlalchemy2 dataclasses

7e566af

rbuffat force-pushed the sqlalchemy2 branch from 4f903f4 to 7e566af Compare March 2, 2023 19:31

expand test matrix

962531f

rbuffat force-pushed the sqlalchemy2 branch from 8e2814c to 962531f Compare March 2, 2023 19:40

rbuffat added 2 commits March 2, 2023 21:25

require py 3.9 for dataclass

cb3ae35

fix sqlalchemy 1.4

2efec56

rbuffat force-pushed the sqlalchemy2 branch from 9ecf6ae to 2efec56 Compare March 2, 2023 20:59

fix test

7154e08

rbuffat force-pushed the sqlalchemy2 branch from 83ff63d to 7154e08 Compare March 2, 2023 21:03

mark test to require python 3.9

8f596eb

rbuffat force-pushed the sqlalchemy2 branch from de6effc to 8f596eb Compare March 2, 2023 21:05

[pre-commit.ci] auto fixes from pre-commit.com hooks

790f98c

for more information, see https://pre-commit.ci

rbuffat changed the title ~~Add support for SQLAlchemy 2.0 Declarative Mapping~~ Add support for SQLAlchemy 2.0 Mar 2, 2023

rbuffat marked this pull request as ready for review March 2, 2023 21:29

revert to typing.List for backwards compatibility

a544669

agronholm reviewed Mar 4, 2023

View reviewed changes

tests/conftest.py Outdated Show resolved Hide resolved

rbuffat added 2 commits March 4, 2023 20:18

Merge remote-tracking branch 'upstream/master' into sqlalchemy2

f454869

add py 3.11

a18d600

fix precommit issues

5ea20c8

rbuffat force-pushed the sqlalchemy2 branch from 58d022e to 5ea20c8 Compare March 4, 2023 19:53

agronholm reviewed Mar 4, 2023

View reviewed changes

rbuffat added 2 commits March 4, 2023 21:06

install sqlmodel

892a98c

install sqlmodel for sqlalchemy 1.4

bbfacf7

rbuffat force-pushed the sqlalchemy2 branch from cc9c293 to bbfacf7 Compare March 4, 2023 20:16

[pre-commit.ci] auto fixes from pre-commit.com hooks

f5b6947

for more information, see https://pre-commit.ci

agronholm reviewed Mar 4, 2023

View reviewed changes

agronholm reviewed Mar 5, 2023

View reviewed changes

rbuffat marked this pull request as draft March 15, 2023 16:39

mhauru mentioned this pull request Jun 21, 2023

Upgrade to sqlalchemy 2 alan-turing-institute/sqlsynthgen#104

Closed

mhauru mentioned this pull request Aug 2, 2023

Add support for SQLAlchemy 2.0, continued #275

Merged

agronholm closed this Aug 3, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for SQLAlchemy 2.0 #249

Add support for SQLAlchemy 2.0 #249

rbuffat commented Feb 5, 2023

This comment was marked as outdated.

rbuffat commented Mar 2, 2023

amacfie commented Mar 2, 2023

agronholm commented Mar 3, 2023

rbuffat commented Mar 3, 2023

agronholm commented Mar 4, 2023

coveralls commented Mar 4, 2023 •

edited

Loading

agronholm Mar 4, 2023

agronholm Mar 4, 2023

rbuffat Mar 4, 2023

agronholm Mar 4, 2023

agronholm Mar 4, 2023

rbuffat Mar 4, 2023

agronholm Mar 4, 2023

mhauru Aug 2, 2023

rbuffat commented Mar 4, 2023 •

edited

Loading

agronholm commented Mar 4, 2023

agronholm Mar 4, 2023

agronholm Mar 5, 2023

mhauru Aug 2, 2023

agronholm Mar 5, 2023

mhauru Aug 2, 2023

agronholm Mar 5, 2023

mhauru Aug 2, 2023

rbuffat Aug 2, 2023

agronholm commented Mar 17, 2023

rbuffat commented Mar 21, 2023

tomeszmh commented May 15, 2023

agronholm commented May 15, 2023

mhauru commented Jul 19, 2023

agronholm commented Aug 1, 2023

mhauru commented Aug 2, 2023

rbuffat commented Aug 2, 2023

agronholm commented Aug 3, 2023

Add support for SQLAlchemy 2.0 #249

Add support for SQLAlchemy 2.0 #249

Conversation

rbuffat commented Feb 5, 2023

This comment was marked as outdated.

rbuffat commented Mar 2, 2023

amacfie commented Mar 2, 2023

agronholm commented Mar 3, 2023

rbuffat commented Mar 3, 2023

agronholm commented Mar 4, 2023

coveralls commented Mar 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rbuffat commented Mar 4, 2023 • edited Loading

agronholm commented Mar 4, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agronholm commented Mar 17, 2023

rbuffat commented Mar 21, 2023

tomeszmh commented May 15, 2023

agronholm commented May 15, 2023

mhauru commented Jul 19, 2023

agronholm commented Aug 1, 2023

mhauru commented Aug 2, 2023

rbuffat commented Aug 2, 2023

agronholm commented Aug 3, 2023

coveralls commented Mar 4, 2023 •

edited

Loading

rbuffat commented Mar 4, 2023 •

edited

Loading